Atespace: per-tenant scoping for the actor lifecycle#280
Atespace: per-tenant scoping for the actor lifecycle#280Haven Xia (HavenXia) wants to merge 6 commits into
Conversation
e9eddfb to
1728f96
Compare
838b199 to
26d0074
Compare
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
Currently an atespace is created merely by using it in an actor. I think we want to move to Atespaces being explicitly created and managed, right? I'm fine with that as a followup, or as more commits in here.
| string name = 1; | ||
| } | ||
|
|
||
| message GetActorRequest { |
There was a problem hiding this comment.
Should we just join name and atespace into a message and use it everywhere? It seems like there's never a context where we want one and not the other?
|
|
||
| // Atespace is the tenant boundary an Actor is created into. Placeholder for now | ||
| // (name only). | ||
| message Atespace { |
There was a problem hiding this comment.
I assumed I would find CRUD APIs for Atespaces, but I don't see them?
| // Lists all known actors. Returns a page of actors and a next page token. | ||
| ListActors(ctx context.Context, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) | ||
| // Lists actors in the given atespace (scoped scan). Returns a page of actors and a next page token. | ||
| ListActors(ctx context.Context, atespace string, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) |
There was a problem hiding this comment.
Does "" mean "all atespaces" ?
| Selector worker_selector = 4; | ||
|
|
||
| // The atespace to create the actor into. | ||
| string atespace = 5; |
There was a problem hiding this comment.
I appreciate the caution of no renumbering proto tags, but we should strive to make this as readable as possible, so I would put it next to ID
There was a problem hiding this comment.
Agree, we are still in a phase, we can break API. Just put a comment in PR that this is a breaking change and all actors needs to be deleted.
In any case, all existing actors will not work, since they will miss namespace.
| func init() { | ||
| createActorCmd.Flags().StringVarP(&templateFlag, "template", "t", "", "Template to derive the actor from in <namespace>/<name> format (required)") | ||
| _ = createActorCmd.MarkFlagRequired("template") | ||
| createActorCmd.Flags().StringVar(&atespaceFlag, "atespace", "", "Atespace (tenant) to create the actor in (required)") |
There was a problem hiding this comment.
Let's avoid the word "tenant"
|
|
||
| createReq := &ateapipb.CreateActorRequest{ | ||
| ActorId: actorID, | ||
| Atespace: at.ObjectMeta.Namespace, |
There was a problem hiding this comment.
This is not obvious to me -- we should never assume k8s namespaces == atespaces -- they are not the same.
We are taking a snapshot, so we should probably use an atespace that is specifically put aside for this. If we move to CRUD of atespaces as a first-class resource, we should "reserve" any atespace whose name being with "ate-", so this can be something like "ate-golden"?
| dnsDomainParts := strings.Split("."+resources.ActorDNSSuffix+".", ".") | ||
| dnsDomainRef := strings.Join(dnsDomainParts, `\.`) | ||
| directives = append(directives, fmt.Sprintf(` match "^%s%s$"`, resources.ActorIDRegexPattern, dnsDomainRef)) | ||
| directives = append(directives, fmt.Sprintf(` match "^%s\.%s%s$"`, resources.ActorIDRegexPattern, resources.ActorIDRegexPattern, dnsDomainRef)) |
There was a problem hiding this comment.
This is fishy -- why do we need an explicit \. in one place but not the other?
Debugger says: the input is ".actors.resources.substrate.ate.dev." (leading and trailing dots). That makes dnsDomainParts have empty first and last entries. That means dnsDomainRef has leading and trailing \.. That makes this correct.
It's WEIRD but correct. I'll send a followup to document it
| // "<actor_id>.<atespace>.actors.resources.substrate.ate.dev" (a trailing dot is | ||
| // tolerated) into its atespace and actor id, validating both. It does not accept | ||
| // a host:port; callers must strip the port first. | ||
| func ParseActorDNSName(name string) (atespace, actorID string, err error) { |
There was a problem hiding this comment.
This seems like a perfect target for a small unit test?
| Selector worker_selector = 4; | ||
|
|
||
| // The atespace to create the actor into. | ||
| string atespace = 5; |
There was a problem hiding this comment.
Agree, we are still in a phase, we can break API. Just put a comment in PR that this is a breaking change and all actors needs to be deleted.
In any case, all existing actors will not work, since they will miss namespace.
| if req.GetAtespace() == "" { | ||
| return status.Error(codes.InvalidArgument, "atespace is required") | ||
| } | ||
| if err := resources.ValidateAtespace(req.GetAtespace()); err != nil { |
There was a problem hiding this comment.
I am curious, why "resources.ValidateAtespace" does not validate for emptry string.
I see similar behavior is defined for resources.ValidateActorID too.
Might be we need to fix all the "resource validator" to check for empty string.
| if req.GetActorId() == "" { | ||
| return status.Error(codes.InvalidArgument, "id is required") | ||
| } | ||
| if req.GetAtespace() == "" { |
There was a problem hiding this comment.
have you considered to have full valudation? resources.ValidateNamespace.
It might worth to add similar full validation for actorID itself.
P.S - please make similar fix for all APIs, if you decide to accept the comment.
This is part of solution for #21.
First incremental slice of the atespace design for actors — part of #21. An atespace is a mandatory tenant boundary that every actor belongs to. It's folded into the actor's identity and storage key (
actor:<atespace>:<id>), so list/get/delete within a tenant is a cheap key-prefix operation, and actors in different atespaces can reuse the same id without colliding.This PR adds atespace through the actor lifecycle end-to-end (proto → store → control API →
kubectl-ate).It doesn't touch DNS, snapshots, scheduling, or auth. Landing it on its own so the future changes are additive, and everything that isn't actor-CRUD is explicitly out of scope below.
This PR changes:
keyed actor:<atespace>:<id>-GetActor/DeleteActor/ListActorstake an atespace. Listing is a scoped SCANactor:<atespace>:*, or SCANactor:*for all atespaces.DNS-1123label. The syncer's dead-worker recovery is atespace-aware by addingWorker.actor_atespace.kubectl-ate:--atespace/-aon every actor subcommand (create/get/delete/resume/suspend/pause/logs).-A/--all-atespacesto list across all tenants.ATESPACEcolumn in the actor table, the existing namespace column is renamedTEMPLATE NSto disambiguate it from the atespace.Examples
Scope a listing to one tenant (-a is shorthand for --atespace):
Creation
Get by atespaces
Resume & Suspend
Scope / non-goals
Deferred to later atespace increments (intentionally not in this PR): the Atespace object + CRUD
RPCs, DNS names, snapshot paths, template grants, the worker's own (system) atespace, and quota.